Fixes #27348: handle Pinot DOUBLE mapping across SQLAlchemy versions#27359
Conversation
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR fixes Pinot column type mapping compatibility across SQLAlchemy 1.4 and 2.x by avoiding a hard dependency on sqlalchemy.types.DOUBLE, preventing runtime ingestion failures reported in #27348.
Changes:
- Add a SQLAlchemy-version-safe fallback chain for Pinot
"double":types.DOUBLE→sqltypes.DOUBLE→types.Float. - Update PinotDB unit tests to accept either
DOUBLEorFLOATfor Pinotdouble, while guarding against integer regressions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
ingestion/src/metadata/ingestion/source/database/pinotdb/metadata.py |
Uses a safe fallback for Pinot double mapping so ingestion won’t crash on SQLAlchemy 1.4. |
ingestion/tests/unit/topology/database/test_pinotdb.py |
Adjusts test expectations for double mapping to be SQLAlchemy-version tolerant while preventing INT regressions. |
| assert result in {"DOUBLE", "FLOAT"} | ||
| assert result != "INT", "Pinot DOUBLE is incorrectly mapped to INT" |
There was a problem hiding this comment.
The custom regression message for INT mapping will never be shown because the preceding assert result in {"DOUBLE", "FLOAT"} fails first when result == "INT". Consider asserting result != "INT" first (with the message) and then constraining the allowed floating types, or folding this into a single assertion so INT regressions produce the intended error output.
| assert result in {"DOUBLE", "FLOAT"} | |
| assert result != "INT", "Pinot DOUBLE is incorrectly mapped to INT" | |
| assert result != "INT", "Pinot DOUBLE is incorrectly mapped to INT" | |
| assert result in {"DOUBLE", "FLOAT"} |
|
Hi @harishkotra! Thank you for your contribution. The change looks good to me although I think the AI suggestions are both good. Could you please update your PR so I can trigger the CI? |
|
Updated per feedback: applied both suggestions (moved DOUBLE fallback resolution to module level and adjusted assertion order in the regression test), then re-ran the targeted Pinot test (10 passed). Ready for CI trigger @edg956 |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
edg956
added
the
safe to test
|
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
@harishkotra could you please review this? |
🟡 Playwright Results — all passed (15 flaky)✅ 3672 passed · ❌ 0 failed · 🟡 15 flaky · ⏭️ 89 skipped
🟡 15 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
|
Updated this PR per review feedback:
Committed and pushed the formatting/base-sync updates @edg956 |
|
@harishkotra there's some infrastructure flakiness in the pytest workflow I'm trying to get past by retrying and there is a playwright test broken from upstream I'm working on right now to unblock this PR |
Appreciate the reponse and thank you for the support. |
Code Review ✅ Approved 1 resolved / 1 findingsUpdates the Pinot DOUBLE mapping logic to resolve inconsistencies across SQLAlchemy versions by processing the type mapping per column. No further issues found. ✅ 1 resolved✅ Performance: double_type resolved inside function called per-column
OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
|
|
@harishkotra thank you for your contribution! Let me confirm whether this will make it to 1.12.6 or the next release |
|
Failed to cherry-pick changes to the 1.12.6 branch. |
Amazing! Absolutely delight to be contributing to this project. Thanks for all the support @edg956 |
Summary
types.DOUBLEhard dependency that breaks on SQLAlchemy 1.4types.DOUBLE->sqltypes.DOUBLE->types.Floatdoubleto allow SQLAlchemy-version-safe floating mapping while preventing integer regressionsValidation
./.venv311/bin/python -m pytest ingestion/tests/unit/topology/database/test_pinotdb.py -q10 passedNotes
AttributeError: module sqlalchemy.types has no attribute DOUBLE).Summary by Gitar
fix(search): scope alias lookups,feat(queryRunner): raise maxResultSize, andfix(domains): fix flaky Rename domain.maininto the feature branch rather than the intent of the PR.This will update automatically on new commits.